Move ldk-server-mcp into the workspace#188
Open
tnull wants to merge 20 commits intolightningdevkit:mainfrom
Open
Move ldk-server-mcp into the workspace#188tnull wants to merge 20 commits intolightningdevkit:mainfrom
ldk-server-mcp into the workspace#188tnull wants to merge 20 commits intolightningdevkit:mainfrom
Conversation
|
🎉 This PR is now ready for review! |
tnull
force-pushed
the
2026-04-move-mcp-into-workspace
branch
2 times, most recently
from
70f8e5f to
3bc8a6e
Compare
Collaborator
benthecarman
left a comment
benthecarman
left a comment
There was a problem hiding this comment.
Did a quick skim and ran through codex
Codex Review:
- [P2] Honor storage.disk.dir_path when resolving credentials — /home/ben/projects/ldk-server/ldk-server-mcp/src/
config.rs:57-60
If the shared ldk-server config uses a custom storage.disk.dir_path, this parser drops that section entirely, so
resolve_config() can only look in ~/.ldk-server/... for api_key and tls.crt. In deployments that store node state
outside the default directory (a setup ldk-server-cli already supports), the MCP bridge will fail to authenticate
unless users also set both LDK_API_KEY and LDK_TLS_CERT_PATH by hand.
- [P2] Fall back to the default gRPC address when no base URL is configured — /home/ben/projects/ldk-server/ldk-
server-mcp/src/config.rs:112-116
In setups that rely on the standard 127.0.0.1:3536 gRPC address and only use the default credential files on disk,
this path exits with “Base URL not provided” instead of using the same default as ldk-server-cli. As a result, ldk-
server-mcp cannot start in the zero-config case unless users create a config file or export LDK_BASE_URL, even
though the server is running at the normal address.
I think these are two things we fixed in the cli since you created the MCP server.
tnull
force-pushed
the
2026-04-move-mcp-into-workspace
branch
from
3bc8a6e to
8d6d7b0
Compare
| @@ -0,0 +1,300 @@ | |||
| // This file is Copyright its original authors, visible in version control | |||
Collaborator
There was a problem hiding this comment.
most of this file seems shared with logic we have in the cli. maybe also worth putting in the client.
| }; | ||
| use serde_json::{json, Value}; | ||
|
|
||
| fn hex_encode(bytes: &[u8]) -> String { |
Collaborator
There was a problem hiding this comment.
we should have hex-conservative available here
| } | ||
|
|
||
| pub async fn call_tool( | ||
| &self, client: &LdkServerClient, name: &str, args: Value, |
Collaborator
There was a problem hiding this comment.
Using serde_json::Value everywhere makes things feel really flaky. I think it'd make sense to bring back the json deserializers for the request types and then use those. That would make things a lot more type safe and less error prone.
| format!("{}:{}", pt.token, pt.index) | ||
| } | ||
|
|
||
| fn build_route_parameters(args: &Value) -> RouteParametersConfig { |
Collaborator
There was a problem hiding this comment.
this file would be hugely cleaned up if we used the proto struct's json deserialzers
| }, | ||
| }; | ||
|
|
||
| let client = match LdkServerClient::new(cfg.base_url, cfg.api_key, &cfg.tls_cert_pem) { |
Collaborator
There was a problem hiding this comment.
should we call get_info to start just to make sure we're connected?
| pub async fn call_tool( | ||
| &self, client: &LdkServerClient, name: &str, args: Value, | ||
| ) -> ToolCallResult { | ||
| for (def, handler) in &self.tools { |
Collaborator
There was a problem hiding this comment.
Would be better to have this as a HashMap keyed by the name so we don't have to iterate through every time.
| if def.name == name { | ||
| return match handler(client, args).await { | ||
| Ok(value) => { | ||
| let text = serde_json::to_string_pretty(&value) |
Collaborator
There was a problem hiding this comment.
Why pretty print it here? This is just going to the agent, so wouldnt it waste tokens
| use serde::Serialize; | ||
| use serde_json::Value; | ||
|
|
||
| pub const PROTOCOL_VERSION: &str = "2024-11-05"; |
Collaborator
There was a problem hiding this comment.
The latest version is 2025-11-25, might be worth updating too. With my brief chat with claude about the differences that'd effect us is that the error handling is slightly improved and more metadata. It also has the remote mcp stuff using SSE but we can ignore that for now.
Comment on lines
+118
to
+120
| let params = request.params.unwrap_or(Value::Null); | ||
| let tool_name = params.get("name").and_then(|v| v.as_str()).unwrap_or(""); | ||
| let tool_args = params.get("arguments").cloned().unwrap_or(serde_json::json!({})); |
Collaborator
There was a problem hiding this comment.
instead of the upwrap_ors here we should be returning a INVALID_PARAMS error
| amount_msat, | ||
| }) | ||
| .await | ||
| .map_err(|e| e.message.clone())?; |
Collaborator
There was a problem hiding this comment.
Here and a lot of others, you don't need to clone the error message
Collaborator
There was a problem hiding this comment.
Instead it actually would make sense to make a converter for LdkServerError to a dedicated error type here. Then we could handle things better like if they server marked the params as invalid vs an actual server error. Then we could actually use INTERNAL_ERROR too
Derive `serde::Deserialize` (alongside the existing `serde::Serialize`) and `#[serde(default)]` on all generated prost messages so that JSON payloads can be deserialized directly into the request types. This reinstates the deserializers that were dropped in 8af3189 so that clients like `ldk-server-mcp` can parse tool arguments straight into the typed proto structs instead of threading every field through `serde_json::Value`. Generated with the assistance of AI (Claude).
Move the MCP bridge into the ldk-server workspace and switch it to an in-tree client dependency so workspace builds and tests cover it directly. Co-Authored-By: HAL 9000
Support storage.disk.dir_path config and fall back to the default gRPC address when neither LDK_BASE_URL nor a config file is present, matching the behavior of ldk-server-cli. Co-Authored-By: HAL 9000
Fix clippy needless_borrows_for_generic_args in test. Co-Authored-By: HAL 9000
Move routing and invoice default constants into ldk-server-client so they are shared between ldk-server-cli and ldk-server-mcp. Co-Authored-By: HAL 9000
Move the shared config loading and credential resolution logic into ldk-server-client so that ldk-server-cli and ldk-server-mcp both consume the same implementation instead of duplicating path handling, TOML parsing, and API-key hex-encoding in each binary. Generated with the assistance of AI (Claude).
Use hex-conservative's DisplayHex implementation for encoding the pathfinding scores blob instead of a hand-rolled hex formatter so the crate matches the hex encoding already used elsewhere in the workspace. Generated with the assistance of AI (Claude).
Dispatch tool invocations through a HashMap keyed by tool name rather than scanning the full registry on every call, and emit compact JSON for tool responses instead of pretty-printing so the MCP bridge doesn't spend tokens on insignificant whitespace when piping results to an agent. Generated with the assistance of AI (Claude).
Probe the ldk-server with a GetNodeInfo call during startup so bad credentials or an unreachable base URL surface right away instead of only being discovered on the first tool call. The probe only logs a warning if it fails so the JSON-RPC loop still serves `initialize` and `tools/list` responses when the server is temporarily down. Generated with the assistance of AI (Claude).
Advertise the `2025-11-25` MCP protocol revision so the bridge reports the current protocol version rather than the older `2024-11-05` spec. Generated with the assistance of AI (Claude).
Classify tool handler errors with a dedicated `McpError` type that maps `LdkServerError` variants onto JSON-RPC error codes (`INVALID_PARAMS` for invalid-request errors, `INTERNAL_ERROR` for auth, Lightning and internal failures) and reuse those codes at the dispatch boundary so a `tools/call` request missing its `name` parameter returns a real JSON-RPC error instead of being silently routed as an "unknown tool". Handlers now propagate `LdkServerError` through a `From` conversion rather than cloning the error message. Generated with the assistance of AI (Claude).
Use the proto-generated `Deserialize` impls to parse each tool's arguments directly into the typed request struct, dropping the per-field `serde_json::Value` scaffolding and the handcrafted helpers for `Bolt11InvoiceDescription`, `ChannelConfig`, `RouteParametersConfig` and page tokens. `expiry_secs` still gets the MCP-side 24h default when omitted; `sign_message` and `verify_signature` keep a manual arg path since their proto `bytes` field cannot be deserialized from a JSON string. Tool JSON schemas are updated to describe the now-nested shapes for invoice descriptions, route parameters, channel configs, and page tokens. Generated with the assistance of AI (Claude).
Use the workspace-built MCP binary directly in tests so regressions are caught without requiring a live server and without recompiling from inside each test. Generated with the assistance of AI (Claude).
Build the MCP binary in the e2e harness and exercise its stdio protocol against a live ldk-server so basic MCP functionality is verified without involving an agent. Co-Authored-By: HAL 9000
Drop the tests/ directory from rerun-if-changed since test changes don't affect the mcp binary. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-04-move-mcp-into-workspace
branch
from
8d6d7b0 to
b76ffeb
Compare
Advertise the bumped `2025-11-25` MCP protocol version in the e2e test's initialize call so its assertion lines up with the server, and pull in the `hex-conservative` dependency that `ldk-server-client` started transitively pulling through its shared config module. Generated with the assistance of AI (Claude).
Run MCP-specific formatting, clippy, and crate tests in a dedicated workflow and add a separate job that exercises the MCP e2e sanity suite on Ubuntu. Co-Authored-By: HAL 9000
Remove redundant formatting check (already covered by the main CI workflow) and drop the MCP e2e job (already covered by the e2e-tests workflow). Co-Authored-By: HAL 9000
Describe the MCP bridge as a first-class workspace member and document how to build, test, and sanity-check it alongside the rest of ldk-server. Co-Authored-By: HAL 9000
Replace exhaustive tool listing with a pointer to tools/list to avoid the maintenance burden of keeping the README in sync with every new RPC. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-04-move-mcp-into-workspace
branch
from
b76ffeb to
b0292ff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moving from https://github.com/tnull/ldk-server-mcp
Draft for now.